Support timezone-aware datetime objects #178

butson · 2025-04-25T16:51:31Z

connection:

Added support for explicit TimeZone connection property

datatype:

Use Julian calendar for date/datetime before 1582 Oct 4. When the Gregorian calendar was introduced: . The day after October 4, 1582 (Julian) became October 15, 1582 (Gregorian). . That 10-day jump is not accounted for in C's calendar logic. . Julian calendar handles leap years differently. The utc calendar the engine is using understands this the python functions based on C calendar logic do not.
time.mktime , time.localtime don't support older dates while engine does changing to use calendar above and calculate time, avoid these issues.
use of datetime + timezone should handle daylight savings times and not be off by an hour.

tests:

Use connection property TimeZone to change timezone
Support timezone-aware datetime
Use pytz correctly with localize if pytz is used
Use zoneinfo.ZoneInfo if available

encodedsession:

Manage and verify TimeZone settings on per connection basis
Allows connections with different TimeZone setting from same application

requirements.txt:

Add requirement for tzlocal, should be available on all supported python versions. There is a change in api between releases , this is handled in the code.

butson · 2025-04-28T16:11:40Z

I expect the cursor test failure is related to the callback to engine to get the real time zone used.

madscientist · 2025-05-12T21:13:59Z

Hi @butson I will try to get to the review proper this week. Just to start out, there are a number of PEP8 violations in the new code.

What I recommend for finding them, if you don't already have an IDE which is capable of enabling flake8 or similar add-ons to do this sort of syntax checking, is to get Visual Studio Code (this is not the full Visual Studio suite, it's a stand-alone, open source editor which runs on all major plaforms: Windows, GNU/Linux, and MacOS): https://code.visualstudio.com/ . Then you should configure it for Python development by installing these extensions: Python (Microsoft), Pylance (Microsoft), and Flake8.

Once you have done that (I recommend restarting after you install the extensions) you can visit the files in your Git repository in the editor, and use CTRL-SHIFT-M to open the "Problems" window and see all the issues reported there. If there are errors not in code you didn't write/update you don't need to worry about it. In particular, the "imported but not used" messages should mostly be ignored, because these tools don't grok the "type:" comments we're using with mypy. But, there are lots of whitespace issues such as, missing spaces after commas etc. in the changed code: these need to be addressed.

madscientist · 2025-05-12T21:19:26Z

Secondly, the error found by circleci in the test_result_set_gets_closed test is due to an incorrect implementation of the new method _set_timezone(). If you write it like this, instead, it will work without errors:

          stmt = self.create_statement()
          self.execute_statement(stmt, "select value from system.connectionproperties where property='TimeZone'")
          rset = self.fetch_result_set(stmt).fetchone()
          self.timezone_name = rset[0]
          self.close_statement(stmt)

Note, you can run these tests locally on your system; you need to set it up but once you do, you can verify changes before you push to GitHub and wait for the CircleCi results. If you're not sure how to do it let me know.

madscientist · 2025-05-15T19:50:26Z

Thanks @butson . Note, there are a lot of mypy and pylint errors as well. You can run make mypy and make pylint to run them. You'll have to install them first (I don't know exactly how to do that on MacOS; I only use Linux myself... let me know if you can't figure it out).

On master, mypy is 100% clean and pylint only complains about a few TODO comments.

madscientist

The thing I'm curious about is that there don't seem to be any NEW tests that test the behavior changes that were made here. It seems like only the existing tests were updated. Put another way, if we kept only the changes to the tests but did NOT add any of the other changes (except the timezone parameter of course), would all the tests pass?

The changes for the timezone parameter are good, but I don't really understand what some of the other changes are accomplishing (like the retrieval of the server zone info etc.) and I was hoping to see some tests that were making that more clear: tests that represented the missing or incorrect facilities and would fail without the changes, and then pass after the changes. Adding/retrieving dates that used to not work, but now work properly, etc.

pynuodb/datatype.py

pynuodb/calendar.py

pynuodb/datatype.py

pynuodb/encodedsession.py

tests/nuodb_basic_test.py

tests/mock_tzs.py

butson · 2025-05-29T16:25:32Z

Paul - I made the changes recommend here. I've updated the branch to just one commit, added test cases, and changed calendar to use the package jdcal. I've run the test on:

Python 2.7.18, 3.8.20, 3.12.8

checked the code with mypy and flake8. I've not been able to run pylint without a bunch of output from code that is not necessarily what I've updated. So any instructions for configuring and running pylint might help if there are issues there.

connection: - Added support for explicit TimeZone connection property datatype: - Use Julian calendar for date/datetime before 1582 Oct 4. When the Gregorian calendar was introduced: . The day after October 4, 1582 (Julian) became October 15, 1582 (Gregorian). . That 10-day jump is not accounted for in C's calendar logic. . Julian calendar handles leap years differently. The utc calendar the engine is using understands this the python functions based on C calendar logic do not. - time.mktime , time.localtime don't support older dates while engine does changing to use calendar above and calculate time, avoid these issues. - use of datetime + timezone should handle daylight savings times and not be off by an hour. tests: - Use connection property TimeZone to change timezone - Support timezone-aware datetime - Use pytz correctly with localize if pytz is used - Use zoneinfo.ZoneInfo if available - added some more tests encodedsession: - Manage and verify TimeZone settings on per connection basis - Allows connections with different TimeZone setting from same application requirements.txt: - Add requirement for tzlocal and jdcal should be available on all supported python versions. There is a change in api between releases , this is handled in the code.

butson · 2025-05-30T22:29:00Z

Paul, test pass now. and ready for your review.

madscientist

Looks pretty good, thanks especially for all the tests. Just a few things to be addressed. I will try to be more timely about future reviews.

madscientist · 2025-06-17T13:26:54Z

pynuodb/connection.py

+            else:
+                # tzlocal < 3.0
+                local_tz = tzlocal.get_localzone()
+                if local_tz:


If this is false, we never set params['timezone'] which means we will get an exception thrown at line 212.

But, looking at the tzinfo definition it seems like it's impossible for get_localzone() to ever return None. So, I think this if-statement at line 210 should be removed.

madscientist · 2025-06-17T13:27:39Z

pynuodb/datatype.py

 from .exception import DataError
+from .calendar import ymd2day, day2ymd
+
+# zoneinfo.ZoneInfo is preferred but not introduced into python3.9


Do you mean "until" rather than "into" in this comment?

madscientist · 2025-06-17T13:37:03Z

pynuodb/connection.py

+                params['timezone'] = tzlocal.get_localzone_name()
+            else:
+                # tzlocal < 3.0
+                local_tz = tzlocal.get_localzone()


Would it be better to use datatype.LOCALZONE here?

madscientist · 2025-06-17T13:41:24Z

pynuodb/datatype.py


 isP2 = sys.version[0] == '2'
+TICKSDAY = 86400
+LOCALZONE = tzlocal.get_localzone()


Why is this always using get_localzone(), while in other places you check the version of python and use get_localzone_name()? Maybe that check should be collected in one place, like here, then everywhere else should use that single assignment...?

madscientist · 2025-06-17T13:42:25Z

pynuodb/encodedsession.py

+        if hasattr(tzlocal, 'get_localzone_name'):
+            # tzlocal >= 3.0
+            self.__timezone_name = tzlocal.get_localzone_name()
+        else:
+            # tzlocal < 3.0
+            local_tz = tzlocal.get_localzone()
+            self.__timezone_name = getattr(local_tz, 'zone')


Wouldn't it work to use datatype.LOCALZONE? I'm not sure why we need to keep repeating this code.

madscientist · 2025-06-17T13:55:00Z

tests/nuodb_date_time_test.py

+
+

Too many blank lines here

madscientist · 2025-06-17T13:56:32Z

tests/nuodb_date_time_test.py

This is great, thanks for adding these tests!

madscientist · 2025-06-17T13:57:11Z

tests/nuodb_date_time_test.py

+
+

Too many blank lines here

madscientist · 2025-06-17T13:58:09Z

tests/nuodb_date_time_test.py

@@ -0,0 +1,428 @@
+# -*- coding: utf-8 -*-
+"""
+(C) Copyright 2013-2025 Dassault Systemes SE.  All Rights Reserved.


This is a new file, from scratch, so your copyright year should just be this year "2025" (no range)

madscientist · 2025-06-17T13:59:42Z

pynuodb/calendar.py

+This uses the Georgian Calendar for dates from 10/15/1582 and
+the Julian Calendar fro dates before 10/4/1582.
+
+(C) Copyright 2013-2025 Dassault Systemes SE.  All Rights Reserved.


I don't think this copyright year is correct. If we wrote this ourselves, rather than copied from somewhere else, then the date should just be this year when we created the file "2025" (no range).

butson requested a review from madscientist April 25, 2025 16:52

madscientist requested changes May 16, 2025

View reviewed changes

butson force-pushed the dbutson/datetime branch 2 times, most recently from 4754ab8 to c808b10 Compare May 29, 2025 16:16

butson force-pushed the dbutson/datetime branch from 6ec0a8d to 83aae13 Compare May 30, 2025 21:46

butson force-pushed the dbutson/datetime branch from 788ff4a to 8d34875 Compare May 30, 2025 22:25

madscientist requested changes Jun 17, 2025

View reviewed changes

Support timezone-aware datetime objects #178

Are you sure you want to change the base?

Support timezone-aware datetime objects #178

Uh oh!

Conversation

butson commented Apr 25, 2025

Uh oh!

butson commented Apr 28, 2025

Uh oh!

madscientist commented May 12, 2025

Uh oh!

madscientist commented May 12, 2025

Uh oh!

madscientist commented May 15, 2025

Uh oh!

madscientist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

butson commented May 29, 2025

Uh oh!

butson commented May 30, 2025

Uh oh!

madscientist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!